Skip to content

fix(falkordb): bound standalone switchover action - #3265

Open
weicao wants to merge 4 commits into
mainfrom
fix/falkordb-switchover-timeout
Open

fix(falkordb): bound standalone switchover action#3265
weicao wants to merge 4 commits into
mainfrom
fix/falkordb-switchover-timeout

Conversation

@weicao

@weicao weicao commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Root cause

A real standalone FalkorDB Switchover requested a specific replica, but kbagent stopped the lifecycle action at its 30s default before the script's 300s convergence check could finish. The live ComponentDefinition had timeoutSeconds: 0, which selects that default.

Simply disabling the kbagent timeout would make the action unbounded and could leave temporary replica priorities behind. Updating the same ComponentDefinition identity also would not regenerate existing Pods' KB_AGENT_ACTION startup environment.

The existing-cluster migration also needs to preserve two contracts explicitly: alpha.1 main Pods must continue resolving an alpha.0 Sentinel sibling, and a timeout-only upgrade must not let KubeBlocks select a different compatible service version or database image.

Changes

  • render standalone Switchover with timeoutSeconds: -1 so kbagent does not preempt Sentinel convergence
  • bound each redis-cli call to 5s plus 2s kill grace
  • supervise the whole switchover script with a 420s timeout plus 60s TERM cleanup grace
  • capture and restore original replica priorities on success, error, EXIT, TERM, and INT
  • restore members in parallel with two bounded attempts per member
  • require exact normalized full-FQDN candidate membership and comparisons
  • wait for every Sentinel to observe the exact candidate priority bias before FAILOVER
  • preserve existing replica-priority=0 on non-candidates
  • fail closed when timeout is unavailable, cleanup fails, or Sentinel never confirms the bias
  • publish the change as new ComponentDefinition identity falkordb-4-1.2.0-alpha.1
  • resolve all six Sentinel cross-component variables through stable major prefix falkordb-sent-4
  • ship an explicit Upgrade OpsRequest pinned to the live serviceVersion
  • verify the same pre/post serviceVersion and exact Pod spec-image/status-image/imageID contract
  • cross-check the old main-Pod Sentinel env contract with exactly one live non-terminating Sentinel sibling, then require the same sibling name/UID and exact env contract after migration
  • require OpsRequest Succeed, exact target ComponentDefinition, complete old-Pod UID replacement, non-terminating Ready replacement Pods, and exactly one serialized switchover action with timeoutSeconds: -1

Verification

Exact head: ebfde18310304ffd1c6044a5c7e373604abc5684

  • independent blocker-fix review: VERDICT: NO BLOCKER
  • Bash 5 FalkorDB ShellSpec: 246 examples, 0 failures
  • Bash 3 compatible suite: 59 examples, 0 failures, 4 declared version skips
  • focused verifier and migration contract: 17 examples, 0 failures
  • production ShellCheck and spec ShellCheck with the repository's existing SC2016 exemption: PASS
  • Bash syntax: PASS
  • Helm lint and structured Helm render: PASS
  • release-note parse and git diff --check: PASS

Runtime focused validation remains owned by Test and must pin this exact commit. These source, mock, and CI results are not a topology runtime PASS.

@weicao
weicao requested review from a team and leon-ape as code owners July 25, 2026 08:57
@codecov-commenter

codecov-commenter commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 419 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (1f914e6) to head (ebfde18).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...lkordb/scripts-ut-spec/falkordb_switchover_spec.sh 0.00% 199 Missing ⚠️
...scripts-ut-spec/upgrade_switchover_timeout_spec.sh 0.00% 144 Missing ⚠️
...cripts-ut-spec/switchover_timeout_contract_spec.sh 0.00% 76 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #3265    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files        163     165     +2     
  Lines      25203   25621   +418     
======================================
- Misses     25203   25621   +418     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread addons/falkordb/scripts/falkordb-switchover.sh
@weicao weicao added the nopick Not auto cherry-pick when PR merged label Jul 31, 2026

@leon-ape leon-ape left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the switchover timeout under the existing falkordb-4-1.2.0-alpha.0 ComponentDefinition identity, but existing Components are not reconciled from ComponentDefinition updates and kbagent reads the action timeout from its startup environment. An in-place addon upgrade can therefore leave existing FalkorDB pods on the old 30-second limit, so the bounded switchover still terminates before Sentinel convergence and targeted switchovers remain unavailable on upgraded clusters.

@weicao

weicao commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the existing-cluster timeout P1 at exact head e1c623d855b358e7d7cc5a1cb36a2ebc4e22e817.

The timeout contract now ships under new ComponentDefinition identity falkordb-4-1.2.0-alpha.1. Existing clusters move through the included Upgrade OpsRequest, which deliberately omits serviceVersion to preserve the current image selection. The included bounded verifier does not trust the OpsRequest phase alone: it requires the exact Component spec.compDef, disappearance of every original Pod UID, non-terminating Ready replacement Pods, and exactly one serialized KB_AGENT_ACTION switchover entry with timeoutSeconds: -1.

Independent focused re-review: VERDICT: NO BLOCKER. Local gates: Bash 5 ShellSpec 236/0; Bash 3 49/0 with 4 declared skips; focused migration/readback 7/0; Helm lint/render, changed-script ShellCheck, Bash syntax, release-note parse, and diff check all pass. Runtime validation remains Test-owned and is not claimed here.

@leon-ape leon-ape left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shipped Upgrade OpsRequest migrates only the falkordb component, while the alpha.1 main ComponentDefinition resolves its Sentinel component, credentials, and pod roster through exact falkordb-sent-4-1.2.0-alpha.1 references. An existing replication cluster keeps its Sentinel sibling on alpha.0, so the recreated main Pods lose those optional Sentinel variables and start via the lexicographically first FalkorDB Pod instead of the authoritative Sentinel primary; after a prior failover this can promote a stale replica and replicate that stale history across the cluster, losing acknowledged writes. The verifier checks only the main Component and Pods, so it reports this mixed-definition topology as successful.

The migration and README treat an omitted serviceVersion as preserving the cluster's current version, but the public Upgrade API defines omission as selecting the latest compatible version; preservation is only the current controller's private nil behavior. A conforming controller change or a newly published compatible version can therefore move the database image during this timeout-only migration, and the verifier still succeeds because it never checks the resolved serviceVersion or image.

@weicao

weicao commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Closed the P0/P1 review findings in exact head ebfde18310304ffd1c6044a5c7e373604abc5684 (tree 71638b76822a285a42f8c0dfbc0e790cdf2d77ad).

  • All six main-to-Sentinel references now use the stable major prefix falkordb-sent-4; KubeBlocks resolves that prefix to the existing alpha.0 Sentinel sibling.
  • The migration manifest explicitly pins serviceVersion: 4.12.5. The verifier rejects a preflight version mismatch and requires the same post-upgrade serviceVersion.
  • The verifier snapshots exact Pod spec images plus status image/imageID values and requires an exact match after Pod replacement, including spec/status container cardinality.
  • Replication topology is fail-closed: old main Pods must agree on the complete six-variable Sentinel env contract, exactly one non-terminating Sentinel sibling must agree with that topology, the same sibling name/UID must remain after upgrade, and replacement Pods must preserve the env contract exactly. Standalone requires neither signal.
  • Adversarial coverage now includes absent, terminating, and disappearing Sentinel siblings, non-empty Sentinel value drift, image-name drift, imageID-only drift, serviceVersion drift, stale Pod UID, conflicting actions, and terminating replacement Pods.

Static/source gates on this head's committed diff: focused verifier+contract ShellSpec 17/0; full FalkorDB ShellSpec Bash 5 246/0; Bash 3 59/0 with 4 declared version skips; Helm lint/render, production ShellCheck, Bash syntax, release-note parse, and git diff --check passed. Independent Dev review of diff SHA-256 850637bef8b6622d94e30a02ba7aa6054049b561dd805efd629190f9e6bdc283 returned VERDICT: NO BLOCKER.

No KubeBlocks runtime migration was executed as part of this source closure, so this does not claim runtime acceptance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nopick Not auto cherry-pick when PR merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants